fix(cohorts): optimized select from cohort_people #21564

mariusandra · 2024-04-16T10:08:58Z

Problem

The following HogQL doesn't always return what you expect:

select person_id, cohort_id from cohort_people

Behind the scenes it's translated to an operation that groups by person_id, cohort_id and with having(sum(sign)) > 0. This works well if the data is correct, but sometimes cohort updates fail leaving messy data. Here's a customer case where sum(sign) equals -16 or -9 or 0 over all rows, even though the last version has all with a +1 sign.

Changes

In "In Cohort" queries we ignore the "sign" field and just check for the latest "version" field. When selecting from the table itself we used just sum(sign) > 0, ignoring the versions.

We'll now do the same we do with 'in cohort' checks: fetch the version information from Postgres and inline it into the query.

Thus this swaps the query select cohort_id, person_id from cohort_people in the following way.

Before:

select cohort_id, person_id from (
    select cohort_id, person_id 
    from raw_cohort_people
    group by cohort_id, person_id
    having sum(sign) > 0
)

After:

select cohort_id, person_id from (
    select cohort_id, person_id 
    from raw_cohort_people
    where (cohort_id, version) in ((1, 2), (3, 4), (4, 5)) and sign > 0
)

I tried inlining the versions in ClickHouse with a (select cohort_id, max(version) as version from raw_cohort_people group by cohort_id) subquery, but on "team 2", this query alone took ~5 sec. Smaller customer teams ran much faster, but this is obviously slow for larger users.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

WIP

neilkakkar · 2024-04-16T10:25:31Z

This is one main reason we moved off of sign and onto version: The collapsing merge tree has shit-resiliency. Ideally nothing uses the cohort sign queries anymore.

I don't think you need to filter on sign either in the new query? If the update is stale (ex: new version has added stuff since you queried for the version from postgres) then you still want to return all data from this version, rather than just the +ve signed data, since the latter may be empty, and the former will be all persons from the previous computation, which is much closer.

And if its the latest version, sign has no meaning anyway

mariusandra · 2024-04-16T10:51:10Z

Thanks for the context!

When I looked around, all previous versions essentially had the same data, but with a -1 sign. I fear without checking for sign > 0, if a version upgrade is taking place while the query is taking place (or some other timing related incident causes us to fetch an older version), we'll suddenly have all person_id-s returned double. To combat this, I should either use a distinct somewhere, or then only check positive versions. Not sure what's faster

neilkakkar · 2024-04-16T11:01:10Z

ohh good point! I think you should go with distinct.

The problem with just sign > 0 is that if some rows have collapsed already (+1, -1 sign merged into an empty one), that person_id will be gone, so it can lead to some random people going in and out of the cohort, which will be very hard to debug, and pretty non-deterministic.

neilkakkar · 2024-04-16T14:03:24Z

posthog/hogql/database/schema/test/test_cohort_people.py

+            team_id=self.team.pk,
+            distinct_ids=["2"],
+            properties={"$some_prop": "something", "$another_prop": "something2"},
+        )


I will like this test more if it also had a third person where $some_prop = "not something"

neilkakkar · 2024-04-16T14:05:59Z

posthog/hogql/database/schema/test/test_cohort_people.py

+            ],
+            name="cohort1",
+        )
+        cohort1.calculate_people_ch(pending_version=0)


Very optional, to make it even more rock solid, you can

Suggested change

cohort1.calculate_people_ch(pending_version=0)

cohort1.calculate_people_ch(pending_version=0)

cohort1.calculate_people_ch(pending_version=2)

cohort1.calculate_people_ch(pending_version=4)

to simulate a few recalculations

neilkakkar · 2024-04-16T14:07:48Z

posthog/hogql/transforms/test/__snapshots__/test_lazy_tables.ambr

-  WHERE equals(cohortpeople.team_id, 420) 
-  GROUP BY person_id, cohort_id, cohort_people___person_id 
-  HAVING ifNull(greater(sum(cohortpeople.sign), 0), 0)) AS cohort_people LEFT JOIN (
+  WHERE and(equals(cohortpeople.team_id, 420), false)) AS cohort_people LEFT JOIN (


false - I'm assuming because wherever this test is hasn't setup the cohort version?

Precisely. We have never stored any data in the cohort, so no need to query anything. Added an extra test to check it as well.

mariusandra · 2024-04-17T07:24:53Z

frontend/src/scenes/debug/HogQLDebug.tsx

@@ -111,7 +111,7 @@ export function HogQLDebug({ query, setQuery, queryKey }: HogQLDebugProps): JSX.
                        <LemonSelect
                            options={[
                                { value: 'auto', label: 'auto' },
-                                { value: 'leftjoin', label: 'join' },
+                                { value: 'leftjoin', label: 'leftjoin' },


Flyby fix 🙈

github-actions · 2024-04-17T07:34:03Z

Size Change: 0 B

Total Size: 999 kB

ℹ️ View Unchanged

Filename	Size
`frontend/dist/toolbar.js`	999 kB

_{compressed-size-action}

mariusandra added 3 commits April 16, 2024 11:54

fix(cohorts): optimized select from cohort_people

4851923

sign

5f27776

no nulls

e850639

mariusandra and others added 5 commits April 16, 2024 13:11

do distinct

8148b11

add test

f1a2f78

no data if no cohorts

6cc97e1

more

2eacb57

Update query snapshots

ac193e1

mariusandra requested a review from neilkakkar April 16, 2024 13:13

mariusandra marked this pull request as ready for review April 16, 2024 13:13

neilkakkar approved these changes Apr 16, 2024

View reviewed changes

mariusandra added 3 commits April 17, 2024 09:20

increase test brutality

0a27db3

use the right term

ade9f4b

null version

e902a78

mariusandra commented Apr 17, 2024

View reviewed changes

mariusandra and others added 3 commits April 17, 2024 09:46

mypy

ad02d11

fix

9a6c1b6

Update query snapshots

d679566

mariusandra merged commit ca49427 into master Apr 17, 2024
104 checks passed

mariusandra deleted the cohort-group branch April 17, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cohorts): optimized select from cohort_people #21564

fix(cohorts): optimized select from cohort_people #21564

mariusandra commented Apr 16, 2024 •

edited

Loading

neilkakkar commented Apr 16, 2024 •

edited

Loading

mariusandra commented Apr 16, 2024 •

edited

Loading

neilkakkar commented Apr 16, 2024

neilkakkar Apr 16, 2024

neilkakkar Apr 16, 2024

neilkakkar Apr 16, 2024

mariusandra Apr 17, 2024

mariusandra Apr 17, 2024

github-actions bot commented Apr 17, 2024 •

edited

Loading

fix(cohorts): optimized select from cohort_people #21564

fix(cohorts): optimized select from cohort_people #21564

Conversation

mariusandra commented Apr 16, 2024 • edited Loading

Problem

Changes

Does this work well for both Cloud and self-hosted?

How did you test this code?

neilkakkar commented Apr 16, 2024 • edited Loading

mariusandra commented Apr 16, 2024 • edited Loading

neilkakkar commented Apr 16, 2024

neilkakkar Apr 16, 2024

Choose a reason for hiding this comment

neilkakkar Apr 16, 2024

Choose a reason for hiding this comment

neilkakkar Apr 16, 2024

Choose a reason for hiding this comment

mariusandra Apr 17, 2024

Choose a reason for hiding this comment

mariusandra Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2024 • edited Loading

mariusandra commented Apr 16, 2024 •

edited

Loading

neilkakkar commented Apr 16, 2024 •

edited

Loading

mariusandra commented Apr 16, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading